Skip to content

Simplify RequestMatcherDelegatingAuthorizationManager.Builder matcher registration #13110

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 22, 2023
Merged

Simplify RequestMatcherDelegatingAuthorizationManager.Builder matcher registration #13110

merged 1 commit into from
Jun 22, 2023

Conversation

@evgeniycheban
Copy link
Contributor

Simplify RequestMatcherDelegatingAuthorizationManager.Builder matcher registration

Closes gh-11624

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 27, 2023
@evgeniycheban evgeniycheban force-pushed the gh-11624 branch 4 times, most recently from dac9fd0 to 652b526 Compare April 28, 2023 00:25
marcusdacoregio
marcusdacoregio suggested changes May 3, 2023
View reviewed changes
Copy link
Contributor

@marcusdacoregio marcusdacoregio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @evgeniycheban, I have left feedback inline.

Can you please ensure that the @since tags are updated to 6.2 because we already have a release candidate for 6.1?

...ingframework/security/web/access/intercept/RequestMatcherDelegatingAuthorizationManager.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The requestMatchers method should keep the same behavior as AbstractRequestMatcherRegistry by creating a MvcRequestMatcher if possible and use AntPathRequestMatcher as a fallback.

spring-security/config/src/main/java/org/springframework/security/config/annotation/web/AbstractRequestMatcherRegistry.java

Line 181 in 1631cac

public C requestMatchers(HttpMethod method, String... patterns) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @marcusdacoregio, thanks for the review. I'm not sure if it would be a good idea to propagate an ApplicationContext to this builder, maybe we could create here AntPathRequestMatcher by default
and let the user register MvcRequestMatcher using requestMatchers(RequestMatcher... matchers).

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right, can you make that behavior explicit in the javadoc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reflected this behavior in the javadoc and updated @since tags to 6.2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @evgeniycheban, I am concerned about having the ant matcher the default if Spring MVC is in the classpath. This deviates from the default that Spring Security adopted in 6.0. For now, what if requestMatchers only takes a RequestMatcher instance? We can think more (in another issue) about how to communicate the MVC nuance to the builder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @marcusdacoregio, makes sense, let's stay only with requestMatchers(RequestMatcher... matchers) for now and think about supporting MvcRequestMatcher in the future releases. I will update the PR soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the PR.

@marcusdacoregio marcusdacoregio self-assigned this May 3, 2023
@marcusdacoregio marcusdacoregio added in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels May 3, 2023
@jzheaux jzheaux added this to the 6.2.0-M1 milestone May 12, 2023
@marcusdacoregio marcusdacoregio mentioned this pull request Jun 22, 2023
Closed
@marcusdacoregio marcusdacoregio merged commit 0cefb27 into spring-projects:main Jun 22, 2023
@marcusdacoregio
Copy link
Contributor

Thanks @evgeniycheban, this is now merged into main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marcusdacoregio marcusdacoregio Awaiting requested review from marcusdacoregio

Assignees

@marcusdacoregio marcusdacoregio

Labels

in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement

Projects

None yet

Milestone

6.2.0-M1

Development

Successfully merging this pull request may close these issues.

Simplify RequestMatcherDelegatingAuthorizationManager.Builder matcher registration

4 participants

@evgeniycheban @marcusdacoregio @jzheaux @spring-projects-issues